fix(codecs): support Avro Date and TimeMillis logical types - #26112
fix(codecs): support Avro Date and TimeMillis logical types#26112omwbennett wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0ca77c6bfb
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4808acf3b0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Head branch was pushed to by a user without write access
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9b6ef3c523
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| other => other, | ||
| }; | ||
| match coerce_logical_types(value.clone(), variant, names) { | ||
| Ok(coerced) if coerced.clone().resolve(resolved_variant).is_ok() => { |
There was a problem hiding this comment.
Resolve union branches with the complete name table
When a union branch is a named record that itself references another named record defined elsewhere, and that nested record contains a date or time-millis field, coercion follows the name table successfully but this guard validates the result against only the dereferenced outer record. That isolated schema cannot resolve the sibling named type, so the coerced branch is discarded; the original Long survives until full-schema resolution and encoding fails. Fresh evidence beyond the earlier fixed nullable-record case is that resolved_variant dereferences only one level, while the new regression test has no nested named reference.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
To fix I think it would be best to use avro-rs 0.22.0's resolve_with_names(). I'll open a separate PR to bump to 0.22.0 then rebase this PR and update to use the new method.
Summary
Support encoding and decoding Avro Date and TimeMillis values.
Due to the way VRL stores integers, an extra step was added to the avro encoder path to coerce Avro types based on the schema field's logical type.
References
Closes #24773
Vector configuration
And similar for TimeMillis.
How did you test this PR?
Manual testing with the above configs, added unit tests for
coerce_logical_typesand enabled the existing date/timemillis avro round trip tests. All tests passing.Is this a breaking change?
Does this PR include user facing changes?
no-changeloglabel to this PR.Contributor Guidelines
@vectordotdev/vectorto reach out to us regarding this PR.pre-pushhook (template) or run the following locally before pushing:make fmtmake check-clippy(auto-fix withmake clippy-fix)make testgit merge origin masterandgit push.Cargo.lock), pleaserun
make build-licensesto regenerate the license inventory and commit the changes (if any). More details on the dd-rust-license-tool.